-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update get_serializer_class
to consider the request method
#77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GenericAPIViewGetSerializerClassTests
tests the changes made in this file.
ListModelMixin, | ||
RetrieveModelMixin, | ||
UpdateModelMixin, | ||
) | ||
|
||
|
||
class GenericAPIView(generics.GenericAPIView): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drf_rw_serializers/generics.py
Outdated
(Eg. admins get full serialization, others get basic serialization) | ||
""" | ||
if hasattr(self, "request"): | ||
if self.request.method in ["GET"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if HEAD, OPTIONS or TRACE would get the write serializer? I imagine they should get the read one, but they don't have a body, so I'm not sure if it would really do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, currently they would get the write serializer. Should I include those values in the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. These should be read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related change: commit 58e95c3. Let me know if there are any adjustments to make to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check comments
Description: Describe in a couple of sentence what this PR adds
get_serializer_class
to consider the request method. See the updated docstring.self.read_serializer_class
.self.write_serializer_class
.self.serializer_class
.Note
Once this PR is approved and merged, I will open another one updating the package version so we can publish it to PyPI.
Dependencies: dependencies on other outstanding PRs, issues, etc.
Resolves #1
Resolves #17
Resolves #44
Merge deadline: List merge deadline (if any)
N/A
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
serializer_class
,read_serializer_class
, andwrite_serializer_class
.Screencast.from.23-05-2024.16.55.32.webm
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.